-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Implement push_mut
#135975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Implement push_mut
#135975
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ibraheemdev (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
Sorry if I did this wrong, it's my first time! |
This comment has been minimized.
This comment has been minimized.
A single trailing whitespace. Oops. |
This comment has been minimized.
This comment has been minimized.
I'm not sure we should be touching |
Alright, I'll do that soon. |
I finally got around to duplicating the implementation. College has been eating my time like a child with Halloween candy, sorry. |
@rustbot review |
library/alloc/src/vec/mod.rs
Outdated
/// | ||
/// let mut vec = vec![]; | ||
/// // Due to current borrow checker limitations (see -Zpolonius), this is the only way to spell this right now. | ||
/// let last = if let Some(v) = vec.last_mut() { v } else { vec.push_mut(0) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the point of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should probably be a better example, yeah. Will update soon!
@balt-dev |
I'm sorry, college has been quite a time sink for me and I haven't had the time to follow up as of recent - I will when I can, promise! |
It's been a rough few months. ._. |
This comment has been minimized.
This comment has been minimized.
hm. |
I think I messed up my fork so bad I'll have to reset it and put my code in from scratch in order to get rid of the merge commit. At least it's copy/pasteable? |
This comment has been minimized.
This comment has been minimized.
This is why I shouldn't program after 9 PM. Will fix when I wake up
tomorrow.
|
@rustbot author |
Reminder, once the PR becomes ready for a review, use |
Failed to set assignee to
|
...is rustbot down? I'm not seeing a reply on my end. |
Ah. Nevermind, GitHub mobile app sucks apparently. |
@rustbot ready |
I think #135975 (comment) still needs to be addressed. If you feel strongly about the API change you can comment on the original ACP or open a new one, but that should probably stay separate from this PR. |
@rustbot ready |
oh |
That's fair. I can go back and make it panic instead, but I'm going to comment on the original ACP first - or make a new one if nobody sees that comment. |
Per some quick discussion on the ACP, I'm going to make it panic, and maybe we can introduce a |
Implementation of #135974.